fix: real-genome correctness — multi-contig eval, contig-naming guard, IUPAC (Act-1 PR-B) - #34
Merged
Merged
Conversation
…, IUPAC Three ways the engine produced wrong results (or crashed) on a real multi-contig human reference while passing on the synthetic demo path: 1. Multi-contig eval (HIGH). eval-germline/eval-somatic loaded only the FIRST FASTA record and compared EVERY variant against it, so a real GIAB run either silently miscompared indels or crashed with an opaque out-of-bounds error. compare_callsets now takes a contig->sequence map and normalizes each variant against its OWN contig (read_fasta_map loads all records); an absent contig errors clearly (names the contig + the available ones), and the normalize out-of-bounds error names the contig. 2. Contig-naming guard (HIGH). A UCSC 'chr1' vs Ensembl '1' mismatch silently dropped every read and wrote an empty VCF with exit 0 — the worst failure mode. validate_contig_lengths now refuses up front when no BAM @sq name resolves into the index, with an actionable message. 3. IUPAC ambiguity (medium). `rosalind index` aborted on R/Y/S/W/K/M/B/D/H/V, which appear in GRCh38's primary assembly and many references. sanitize_ reference maps them to N (matching bwa/bowtie); genuinely invalid bytes still error. Tests: multi-contig eval + naming-mismatch (integration), disjoint-naming guard + IUPAC-to-N (unit). compare_callsets call sites updated to the map API. Full suite green; rustc 0 warnings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Act-1 hardening, PR-B (real-genome correctness). Three ways the engine produced wrong results or crashed on a real multi-contig human reference while passing on the synthetic single-contig demo path — the §B findings from the reflection audit.
1. Multi-contig
eval(HIGH)eval-germline/eval-somaticloaded only the first FASTA record and compared every variant against it. On a real GIAB benchmark that means either silently corrupted indel TP/FP/FN or an opaque out-of-bounds crash — on the project's headline truth-comparison surface (and directly contradicting the README's "GIAB-ready" claim, which this PR makes true).compare_callsetsnow takes aBTreeMap<String, Vec<u8>>and normalizes each variant against its own contig;read_fasta_maploads all records.NormalizeError::OutOfBoundsnow names the contig.2. Contig-naming guard (HIGH)
A UCSC
chr1vs Ensembl1mismatch silently dropped every read, wrote a header-only VCF, and exitedcontract: OK— the worst failure mode (a clean run that called nothing).validate_contig_lengthsnow refuses up front when no BAM@SQname resolves into the index, with an actionable message.3. IUPAC ambiguity codes (medium)
rosalind indexaborted onR/Y/S/W/K/M/B/D/H/V— which appear in GRCh38's primary assembly and many bacterial/viral references — so the pipeline's entry point couldn't ingest a stock reference.sanitize_referencenow maps them toN(matching bwa/bowtie); the N-mask carries the ambiguity, and genuinely non-sequence bytes still error.Test plan
tests/eval_multicontig.rs— a chr2 variant past chr1's length compares correctly (2 TP) instead of crashing; a variant on an absent contig errors clearlystreaming_source_rejects_disjoint_contig_naming(unit) — Ensembl1BAM vs UCSCchr1index refusessanitize_maps_iupac_ambiguity_codes_to_n(unit) — IUPAC→N, invalid bytes still rejectedcompare_callsetscall sites updated to the map API (clinical_eval_gates,germline_accuracy)cargo fmtcleanAct-1 sequence
PR-B of 3. PR-A (#33, soundness — predicted peak a true upper bound) is open and CI-green. Remaining: PR-C (trust on-ramp + hygiene — Action snippet,
install.shchecksum, CI tautology,verifyself-hash, clippy gate). Independent branches offmain(disjoint surfaces), mergeable in any order. Plan:docs/superpowers/plans/2026-06-02-act1-contract-hardening.md.🤖 Generated with Claude Code